Conversation
Katzenkralle
force-pushed
the
invide_confirmation
branch
from
07ebbe2 to
dc9cc40
Compare
Katzenkralle
force-pushed
the
invide_confirmation
branch
from
cb3637d to
9d09797
Compare
|
Why is this not merged yet? Would be super handy. |
Hi @pascalmtts like many OSS projects, we do our best to review and maintain our codebase. This project is especially sensitive since it’s security-related, so we review changes more carefully than usual. I’ll start the review later this week. On a personal note, have you had a chance to test this MR in an environment? Any feedback would also help move the merge forward. |
| method, path, headers=headers, **kwargs | ||
| ) | ||
|
|
||
| def get_public_key_for_user(self, user_id: UUID | None = None) -> str: |
There was a problem hiding this comment.
get(publickey) returns None if the key is missing, but the return type is str. This None will then be passed to the others functions (b64) and will will raise an opaque excepton. You should explicitly check and raise a meaningful error (e.g. BitwardenError("User has no public key")).
| ) | ||
|
|
||
| def get_public_key_for_user(self, user_id: UUID | None = None) -> str: | ||
| used_id = user_id if user_id else self.sync().Profile.Id |
There was a problem hiding this comment.
Can you kept the same naming variable user_id pls
| def confirm( | ||
| self, | ||
| new_user: OrganizationUserDetails, | ||
| ): |
There was a problem hiding this comment.
can you add a docstrings pls and return type is not annotated pls
| new_user: OrganizationUserDetails, | ||
| ): | ||
| rsa_public_key_new_user = b64decode( | ||
| self.api_client.get_public_key_for_user(new_user.UserId) |
There was a problem hiding this comment.
If it's None here, get_public_key_for_user(None) falls back to
self.sync().Profile.Id )the calling user's own public key?)
This means the organization key would be encrypted for the wrong user, potentially leaking it ?
| resp = self.organization.invite("test-account-3@example.com") | ||
| self.assertTrue(resp.is_success) | ||
|
|
||
| if not os.environ.get("VAULTWARDEN_INVITATIONS_ALLOWED", "True").lower() in ["true", "1", "yes"]: |
There was a problem hiding this comment.
VAULTWARDEN_INVITATIONS_ALLOWED is already explicitly set in the test script, do we really need to recheck it here?
Not related to this MR, we consider refactoring the entire test setup so we can more easily handle cases where we want to run the same tests with different variables. Ideally, this would integrate cleanly with pytest.mark and testcontainers.
3 participants
When Vaultwarden is configured with
INVITATIONS_ALLOWED="false", users invited to an organization must be confirmed before their access to the organization is granted.This adds a
.confirm(user)method to theOrganizationclass that allows us to do that.